-
-
Notifications
You must be signed in to change notification settings - Fork 862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf!: cache projection of polygon points & CRS improvements #1801
Conversation
965678d
to
5986b39
Compare
Cool! Will try out the PR with my CRS for a better feel soon, just adding some initial thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, some great changes, thanks :) Few minor changes, and I'll make a couple basic formatting changes.
After the research into Record
s which revealed they were innefficient, should we be using them? I believe you were there or acknowledged it, but I'll try to dig it out if not.
This PR appears to be split into two parts, CRS improvements and Polygon improvements. Should they be seperated into seperate PRs? (no worries if that's too time consuming.) |
Yeah, pretty much. I was torn. I started with a Record (especially since some of the CRS methods already use records) but at the end:
Though the last points is something we should certainly discuss more in terms of safety if we decide to make PointDouble public. At the moment I'm only really using it internally, i.e. we can change it w/o a major version change.
Once could, though I'd argue that CRS changes are very minor (it's mostly just adding (un)transform). If we'd split them we'd add new APIs w/o a user. |
Thanks for the reformatting. Quick remark, if you guys feel strongly about style i'd suggest installing or writing linter rules for it. Overall that will reduce churn, enforce the preferences more consistently across the code-base, and tell authors what you're looking for. |
b30cdcd
to
1990204
Compare
Yeah we've been slowly adding them (although not for a while). One I feel strongly about is trailing commas, but I know that's debated :D |
4e9ede3
to
45d194c
Compare
Ok, so I think this just needs the conversation with @JosefWN resolved, and the same changes as applied to the polygons applied to the polylines as well (as I believe some of the changes have affected polyline simplification, but the other changes have not been made). |
Feel free to weigh in, always welcome a tie breaker. As far as I can tell, there are two conversations interleaved:
Personally, I'm happy to do any rename, however feel strongly about not using math.Point especially in internal APIs since it goes against the performance optimizations we're trying to do here 🤷
This change should not affect polyline. We should be able to make these changes independently. |
I'll opt out of discussing this one, I'm not even remotely qualified :D
I think you're right, given that the entire point of these changes is performance, we might as well squeeze as much out of this as possible. It's not ideal, and could be confusing, but it's documented well enough.
I'm not sure whether it affects polylines directly - the |
2bb0671
to
8d08933
Compare
Nothing should have changed for polylines. Simplify is unit agnostic. At the moment:
I was hoping to do polylines in a follow up. WDYT? |
Ah OK, my misunderstanding, in that case I'm happy for it to be delayed out of this PR, but it does need to be done before the v7 release. |
Perhaps at least a doc comment stating the intended purpose of the class? But I will leave this PR in the hands of the maintainers, there isn't that much more I can add. |
That does seem reasonable. |
…and polyline hit detection code.
…ser-provided tolerance will have similar meaning independent of the user-selected projection.
…tate CrsWithStaticTransformation as internal.
c04c387
to
d6ff03f
Compare
Added a docstring, annotated it explicitly internal, and am not exporting it. Hope that addresses any concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I committed e170a1a and 9e07d1f. I removed the doc strings because they are internal functions, and have relatively self documenting names and bodies:
I'm happy to add them back in another PR @josxha, when I/we more strongly revise the documentation. |
If you want to documentation in a follow up pull request it's fine for me - althrough it shouldn't get delayed to much or someone unlucky in the future needs to dig through these complex undocumented functions. |
RFC based on #1793